Conversation
|
hmm there's definitely some strange resolution stuff happening in node 24. http 6 seems to be broken (even on a basic node.js import). node 22 is fine and http v8 is fine. What's happening in http6 and node 24? Nothing in the migration guide which might explain this |
|
I can see that the exports from the modules are the same. common functions work great. But all http functions fail. |
|
this problem sort of exists anyway. If I go into adaptors and run from a local build, using production openfn and node 24, I get this same error: if I switch to node 22 in the adapors repo and re-run the cli, it works great |
| // Safer for now to just use the CJS import | ||
| if (pkg.main) { | ||
| main = pkg.main; | ||
| // Find the best ESM entrypoint |
There was a problem hiding this comment.
Scary change here
It looks like the problems I'm (luckily!) catching in tests is that node24 has changed the module loader a bit and support for CJS feels flaky. That's OK, I'd much rather be using ESM anyway.
So this change prefers the ESM import to the CJS one.
I just worry about whether things are going to break?
There was a problem hiding this comment.
Yes, some things break. Some older packages fail to import through the ESM loader (basically if they import paths which don't have a .js extension)
After working with claude and closely tracking the docs
|
I think integration tests fails are just flaky now. I don't understand the unit test fails - we seem to get a SIGTERM half way through the tests |
|
Seeing lots of errors like this creeping out of tests: What's happening is we've switched to loading modules in the old CJS format and preferring the new ESM format. But there are subtleties in the import which means that some import statements inside our dependencies no longer work - like the above one coming out of What concerns me is that import statements we are not in control of are causing the runtime to crash. It's mostly old versions but still, I don't want things to start suddenly breaking in the worker. The old runtime import, the CJS import, works on both node 22 and 24. The new import breaks with these particular patterns on node 22 and 24. |
| [statePath]: state, | ||
| [outputPath]: '{}', | ||
| [pnpm]: mock.load(pnpm, {}), | ||
| [recastPath]: mock.load(recastPath, {}), |
There was a problem hiding this comment.
bonus: this will make tests run quicker. Should I do this in more places?
| } | ||
| ); | ||
|
|
||
| test.serial('pull: should pull a simple project', async (t) => { |
There was a problem hiding this comment.
Removed this because it's failing, but we have better coverage of this in integration tests now
…1343) * add a custom module loader to handle older imports without .js extensions * types * typo * fix module loader path * fix module import and update tests * use esmhook in metadata * remove stupid logging * remove debug flag from tests
Hoping this will fix failures in CI
2 participants
Short Description
Fixes #1326
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy